added flag for clearing log files at restart #3203

seun-ja · 2025-07-18T16:52:27Z

Aims to fix #3130

This PR introduces a new flag truncate-logs. It is used with the up command like this:

spin up --truncate-logs

Test

Due to the complexity related to testing it. It was manually tested.

Initially, ran the default spin up after building the spin app, the log files (test_component_stdout.txt & test_component_stderr.txt) were populated with logs, reran it, and checked the logs; the logs from the previous run were still there.

Finally, ran spin up --truncate-logs, checked the logs, and they were empty.

itowlson · 2025-07-20T19:35:20Z

crates/trigger/src/cli.rs

@@ -54,6 +55,14 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde
    )]
    pub log: Option<PathBuf>,

+    /// Whether to refresh logs when restarted.


I found the word "refresh" a bit odd here, although I can see the connection when I squint. At least in help text, I think we should be more explicit about what the flag does, e.g.

Suggested change

/// Whether to refresh logs when restarted.

/// If set, Spin deletes the log files before starting the application.

(or 'clears the log files'? Not sure which is better)

itowlson · 2025-07-20T19:35:41Z

crates/trigger/src/cli.rs

+    /// Whether to refresh logs when restarted.
+    #[clap(
+        name = REFRESH_LOGS,
+        short = 'r',


I wouldn't spend a valuable shortcode on this.

itowlson · 2025-07-20T19:38:10Z

crates/runtime-config/src/lib.rs

@@ -192,6 +198,8 @@ pub struct TomlResolver<'a> {
    state_dir: UserProvidedPath,
    /// Explicitly provided log directory.
    log_dir: UserProvidedPath,
+    /// Whether to refresh logs when restarted.
+    refresh_logs: bool,


A bit strange to see this in a struct called TomlResolver - it's not like it has anything to do with resolving TOML. But I guess maybe the log directory is already non-TOML-related and the struct has just grown beyond its original name? Not sure.

Somewhat, yes.

All but refresh_logs still source the default value from table: TomlKeyTracker<'a>, so the initial name makes sense. However, with other elements added, a change of name would be better, something like RuntimeConfigurationResolver. Would that be fine, or what would you suggest?

I should have come back to this comment after writing the one about the location of the delete code - sorry.

We should first figure out a good place for the delete code to live, whether in this struct impl or elsewhere. Then that will guide us as to whether this flag needs to live here or not.

itowlson · 2025-07-20T19:44:28Z

crates/runtime-config/src/lib.rs

@@ -268,7 +278,17 @@ impl<'a> TomlResolver<'a> {
        }

        match log_dir {
+            UserProvidedPath::Provided(p) if self.refresh_logs => Ok(Some({
+                let _ = std::fs::remove_dir_all(&p);


This seems to end up duplicating the check and the deletion code. It seems like something you could do outside the match, if the match successfully resolves a path.

But I see this is in a function whose job is to "get the configured log directory." Getting path to a directory should not have a potential side effect of deleting that directory - that's super surprising behaviour.

(Similarly, notice that this function does not create the directory. It just gets the path.)

So I think this code needs to move somewhere else, at which point hopefully we will know where the log directory is (if we have one) and can clear it without resorting to repeated guards or code.

itowlson · 2025-07-20T21:01:07Z

crates/trigger/src/cli.rs

@@ -54,6 +55,14 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde
    )]
    pub log: Option<PathBuf>,

+    /// Whether to refresh logs when restarted.
+    #[clap(
+        name = REFRESH_LOGS,


Maybe consider a SPIN_ prefix for this? Going by the original issue, some folks will be treating this as a user preference and setting it in .profile rather than on the spin up command line, so it would be good to have it make sense out of the Spin context perhaps?

(sorry forgot to think of this while reviewing)

itowlson · 2025-07-21T01:25:16Z

crates/runtime-config/src/lib.rs

@@ -147,6 +143,11 @@ where

        let toml = toml_resolver.toml();
        let log_dir = toml_resolver.log_dir()?;
+
+        if refresh_logs {
+            let _ = Self::resolve_log_dir(&log_dir);


I'm sorry to be a nag but once again this is kind of jamming the delete code in somewhere that yes, it's convenient right now because this code knows what the log directory is, but will be surprising to anyone debugging in a few months. This is the constructor for the runtime configuration. It has nothing to do with creating or deleting directories.

What I really think we should do before diving into writing code is some design:

What is the desired behaviour of the feature?

What considerations do we have to think about when implementing the feature?

What code object(s) are responsible for the behaviour?

Let me give some examples.

The current behaviour is to delete the logs directory. Suppose, for some reason known only to myself, I set the logs directory to be my application directory (spin up --log-dir . --refresh-logs). Oops, Spin just deleted my application directory. Should the behaviour be to delete individual log files rather than the whole directory?

The current implementation deletes the directory as part of trigger startup. That means that if I have a multi-trigger application, each trigger will perform a directory delete as it starts. Suppose one trigger fails early, and another trigger starts slowly. The second trigger blew away my logs for the failure.

The existing log file creation happens in ComponentStdioWriter, in the spin_trigger::cli::stdio module. Does deleting existing logs fit nicely with the existing responsibilities of that type or module? I'm not saying it does, I'm just giving it as an example of a place I'd look at before the runtime-config subsystem.

I don't know if there are other factors to consider because I've not done any design on this, I just feel like this might run smoother if you were gathering feedback on a holistic proposal rather than trying to make local tweaks in response to a stream of comments. That said, this is your project and if you feel more comfortable working at the code level then no worries!

Taking a further look at this issue in the broader context (apologies for not doing this earlier), I am struggling to find an intersection between up's subsequent calls/methods and trigger's subcommand where refresh-log and log-dir can be accessed.

Here's an approach I'm considering

Ideally, the refresh should be done before the triggers are started when the up command is called
https://github.com/seun-ja/spin/blob/9d09742a860ffea180d3eadd547424c41b577312/src/commands/up.rs#L239

handling it in ComponentStdioWriter could have been idle, as it has a connection to trigger commands, it wouldn't work because it is called on every request, hence if placed there, it deletes and recreates the file, not ideal

Would you be open to a pair?

Good catch on ComponentStdioWriter and sorry for the bad steer. I do think that module as a whole will repay attention. StdioLoggingExecutorHooks is the other half of the equation, and that does get to participate in trigger startup. Looking at other hooks (e.g. InitialKvSetter), configure_app is where they do their "do once at startup" work - we'd have to figure out the list of log files relevant to the trigger, and I'm not sure if the ConfiguredApp is pared back to just the trigger components at this point. But testing will clarify that (or more code reading!).

itowlson

Thanks for this - it looks good! I had a couple of style and naming nits, which are non-blockers but I'd be keen to see in; but my only remaining functional concern is the multi-trigger-type scenario. If you could validate the each trigger only truncates its own log files then that would be very reassuring!

Indeed, generally, since this is hard to write unit tests for, it would also be very useful to have some notes in the PR about what you tested and how.

Thanks again for persevering with this!

itowlson · 2025-07-22T20:18:25Z

crates/trigger/src/cli/stdio.rs

@@ -328,3 +369,11 @@ fn bullet_list<S: std::fmt::Display>(items: impl IntoIterator<Item = S>) -> Stri
        .collect::<Vec<_>>()
        .join("\n")
 }
+
+fn santized_log_path(


Suggested change

fn santized_log_path(

fn sanitized_log_path(

itowlson · 2025-07-22T20:25:20Z

crates/trigger/src/cli/stdio.rs

+                    STDOUT_LOG_FILE_SUFFIX,
+                ) {
+                    if let Err(err) = std::fs::File::create(stdout_log_path) {
+                        //TODO: figure out if we want to return error here


As you have it now, create will fail if the logs dir doesn't already exist, and that shouldn't be considered an error or warning. But you might want to move it to after the "create the log directory" bit so that it doesn't fail...

itowlson · 2025-07-22T20:26:40Z

crates/trigger/src/cli/stdio.rs

@@ -95,6 +103,39 @@ impl<F: RuntimeFactors, U> ExecutorHooks<F, U> for StdioLoggingExecutorHooks {
        configured_app: &spin_factors::ConfiguredApp<F>,
    ) -> anyhow::Result<()> {
        self.validate_follows(configured_app.app())?;
+
+        if self.refresh_log {


This is a very long block which now dominates the configure_app function. Consider breaking out e.g. if self.refresh_log { self.truncate_log_files(); }

itowlson · 2025-07-22T20:27:53Z

crates/trigger/src/cli/stdio.rs

+        if self.refresh_log {
+            let sanitized_component_ids: Vec<String> = configured_app
+                .app()
+                .components()


Did we confirm that .components() returns only the components being run by this trigger?

Yes, returns only this trigger

itowlson · 2025-07-22T20:31:49Z

crates/trigger/src/cli.rs

+    /// If set, Spin deletes the log files before starting the application.
+    #[clap(
+        name = SPIN_REFRESH_LOGS,
+        long = "refresh-logs",


I'd really like to align on the language here. Is the consensus for "refresh logs"? It feels ambiguous to me and I'd prefer "clear logs" or "truncate logs" or something. (But if we do make a change then the language needs to line up everywhere, not just on this line - field names, EVs, docs, etc.)

itowlson · 2025-07-22T20:32:53Z

crates/trigger/src/cli.rs

@@ -54,6 +55,13 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde
    )]
    pub log: Option<PathBuf>,

+    /// If set, Spin deletes the log files before starting the application.


Given the proposed implementation, consider e.g.

Suggested change

/// If set, Spin deletes the log files before starting the application.

/// If set, Spin clears the log files before starting the application.

(since we no longer delete them)

itowlson · 2025-07-22T20:33:35Z

crates/trigger/src/cli.rs

@@ -139,6 +147,8 @@ pub struct FactorsConfig {
    pub follow_components: FollowComponents,
    /// Log directory for component stdout/stderr.
    pub log_dir: UserProvidedPath,
+    /// If set, Spin deletes the log files before starting the application.


Suggested change

/// If set, Spin deletes the log files before starting the application.

/// If set, Spin clears the log files before starting the application.

itowlson

Looks good - thanks! It would be great if you could write something about testing but the code looks fine!

itowlson · 2025-07-22T23:13:38Z

crates/trigger/src/cli/stdio.rs

+                &sanitized_component_id,
+                STDOUT_LOG_FILE_SUFFIX,
+            ) {
+                let _ = std::fs::File::create(stdout_log_path);


I think you don't need the let here - the _ suffices as a discard indicator.

Suggested change

let _ = std::fs::File::create(stdout_log_path);

_ = std::fs::File::create(stdout_log_path);

(I may be mis-remembering though)

itowlson · 2025-07-22T23:13:53Z

crates/trigger/src/cli/stdio.rs

+                &sanitized_component_id,
+                STDERR_LOG_FILE_SUFFIX,
+            ) {
+                let _ = std::fs::File::create(stderr_log_path);


same comment (and same disclaimer!)

- added flag for clearing log files at restart Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - refactor: renaming + login Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - refactor: creates instead of delete Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - warn of error Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - revert format Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - some renaming + cleaner code Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com> - clean up Signed-off-by: Aminu 'Seun Joshua <seun.aminujoshua@gmail.com>

itowlson requested changes Jul 20, 2025

View reviewed changes

itowlson reviewed Jul 20, 2025

View reviewed changes

itowlson reviewed Jul 21, 2025

View reviewed changes

seun-ja requested a review from itowlson July 22, 2025 20:09

itowlson reviewed Jul 22, 2025

View reviewed changes

seun-ja force-pushed the clear-log-files-when-re-building-locally branch from 254bf4d to 41a7783 Compare July 22, 2025 22:19

seun-ja requested a review from itowlson July 22, 2025 23:03

itowlson approved these changes Jul 22, 2025

View reviewed changes

seun-ja force-pushed the clear-log-files-when-re-building-locally branch from 3bcb269 to 1edd2d5 Compare July 23, 2025 09:58

	/// Whether to refresh logs when restarted.
	/// If set, Spin deletes the log files before starting the application.

	/// If set, Spin deletes the log files before starting the application.
	/// If set, Spin clears the log files before starting the application.

	let _ = std::fs::File::create(stdout_log_path);
	_ = std::fs::File::create(stdout_log_path);

added flag for clearing log files at restart #3203

Are you sure you want to change the base?

added flag for clearing log files at restart #3203

Conversation

seun-ja commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itowlson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seun-ja commented Jul 18, 2025 •

edited

Loading